feat: Add cachedimage pulling progress status #402
Conversation
|
Thanks for this new PR ! It will be easier for me to review since there are less commits and much less changes too haha To answer your comment in the previous one (by the way, you could also squash, reword and force-push your old branch instead of creating a new one, but, this way we keep discussions in the same PR, but it's ok for this time don't worry): I think we don't need to bump the version of the CRD, the goal of naming it I will review it soon, thansk again for your work! |
There was a problem hiding this comment.
I tried to compile and run your changes on my dev cluster and I'm a bit confused about the values showed in the status of CachedImages. For instance for quay.io/jetstack/cert-manager-cainjector:v1.13.3, it shows a total of 318 (bytes ?) but docker inspect quay.io/jetstack/cert-manager-cainjector:v1.13.3 | jq '.[0].Size' outputs 42445613. So it doesn't really seems to work. Also showing the total size doesn't seems that useful to know the progression, as I would have to download and inspect the image first, or maybe describe the CachedImage in order to compare the status.progress.available vs status.progress.total. It would be much more interesting to have a percentage of completion instead of a total.
PS : conventional commit requires commits message to not start with an uppercase letter
I'm also puzzled by this. Here is the experiment I've done (https://mcr.microsoft.com/en-us/product/dotnet/sdk/tags)
Not sure what is happening under the hood from |
| @@ -348,16 +348,11 @@ func (r *CachedImageReconciler) cacheImage(cachedImage *kuikv1alpha1.CachedImage | |||
| lastWriteComplete := int64(0) | |||
| onUpdated := func(update v1.Update) { | |||
| needUpdate := false | |||
There was a problem hiding this comment.
Thanks for the update, but now the code isn't compiling...
Error: internal/controller/kuik/cachedimage_controller.go:350:3: needUpdate declared and not used
Sorry but I cannot merge something that don't work as expected and I'm a bit worried that you implemented a feature without understanding what you were doing. If you really need this feature please dig a bit more into the functioning of this in order to get a clear understanding of it and make it work as expected. |
Add following property in CachedImage CRD
spec.progress.availablespec.progress.totalUpdate the
status.progress.availablein CachedImage CRD every 5 seconds during the image pulling in cachedimage controllerThis is to replace the PR #400 with all the clutters removed.
This is to implement the feature request #401
The CRD / API version is not changed here, still left as
v1alpha1.